Repository navigation
fix(codegen): reuse SignalListing.Range in SignalListing subclasses by shape, and fail loudly on a miss - #1891
Conversation
…n a copy survives fix_signal_listing_range_subclasses matched the generated Range text, extra='forbid' and all. The generator moved to StrictFloat and new descriptions, so the match stopped deleting the duplicate classes in signals/get_signals_response.py and core/wholesale_feed_event.py, and still printed success. Its import rewrite still fired, and remove_imports_shadowed_by_a_local_class then undid it, so the post-generation manifest saw both passes firing. adcp#8866 opens core/signal-listing.json#/properties/range, which would have kept it missing. The pass now finds each copy by name and compares it with SignalListing.Range by what it validates (bases, model_config, field types, constraints, defaults; not descriptions). On a match it binds Range to the shared class by assignment, so adcp.types.domains.signals.Range stays exported. It raises when a copy validates differently or a Range class survives. Regenerated on 3.3.0-beta.5: SignalListing subclasses now share SignalListing.Range, which also removes the parent/override type mismatch on `range`. Checked against adcp#8866's schema bundle (open range), where the pass applies to both modules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the public API snapshot SignalCoverageRange and adcp.types.domains.signals.Range now resolve to the shared core.signal_listing:Range, the intended effect of the reuse. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Clean review with no Medium-or-worse findings. The PR rewrites fix_signal_listing_range_subclasses in scripts/post_generate_fixes.py from brittle text-matching to an AST shape comparison against SignalListing.Range (fail-closed raise on divergence at :185-191; local class replaced with an assignment at :200 so consolidate_exports still re-exports a declared Range).
No public-API break: get_signals_response.Range becomes the same object as signal_listing.Range, so aliases.py's Range as SignalCoverageRange import still resolves. The snapshot move is __module__-only — same shape (extra='forbid', StrictFloat), no wire change, so no breaking semver signal or changeset is required. Manifest flip to a string reason is consistent and self-graded by _check_manifest. New test file tests/test_signal_listing_range_reuse.py (272 lines) covers divergence, idempotency, parenthesized-import, and the integration identity assertion.
Decision table: no critical/high/medium findings; gated_paths false; high_risk false; no author team gate; no prior decision. Falls through to row 9 → approve.
…s SignalListing The earlier commit aliased Range by assignment in two named files. That broke the domains invariant that a module binds only classes it defines (test_no_generated_type_is_reachable_under_zero_names). It also changed what signals/get_signals_response.Range is, though that module stopped subclassing SignalListing when 3.3 inlined its signal, so there is no parent type to match. The pass now scans every generated module for a SignalListing subclass that declares Range, deletes that copy when it validates like SignalListing.Range, and adds Range to the module's existing signal_listing import. It raises on a divergent copy, on an import it cannot extend, and on a surviving duplicate. get_signals_response keeps its own Range, so the public API snapshot and adcp.types.domains.signals.Range are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bokelley
changed the title
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Checked: the delta refines fix_signal_listing_range_subclasses (post_generate_fixes.py:5231-5240) to reuse SignalListing.Range only where a module actually subclasses SignalListing, leaving inlined get_signals_response with its own Range. Logic is fail-closed on divergence, attribute-base-without-direct-import, and surviving duplicates, covered by 19 tests. The add_to_import module reconstruction (5279) matches the import pattern. The SignalCoverageRange/Range snapshot repoint is shape-identical (min/max StrictFloat, closed) with aliases.py unchanged — a correction, not a semver break, so no breaking-change commit prefix is required.
One non-blocking note (Low, omitted from blockers): the PR body's "public API snapshot is unchanged" claim is stale since the snapshot repointed two entries, but the underlying change is sound.
Decision path: no critical/high/medium findings; high_risk false; gated_paths false (and review_decision is APPROVED regardless); no no-auto-approve team match. Rows 1–8 do not fire → row 9 approve. Prior decision was also approve; this is a net-neutral/clean continuation.
KonstantinMirin
left a comment
KonstantinMirin
left a comment
There was a problem hiding this comment.
Hi Brian, thanks for this. I looked at b44fe3287f. Your later commit drops the Range = _SignalListingRange alias, which also clears the export-surface test failure I saw at 7b8520eab.
The main thing: I'd rather fix this in the schema and delete the pass than make it sturdier. signal-listing.json#/properties/range, get-signals-response.json#/properties/signals/items/properties/range and wholesale-feed-event.json#/$defs/signal/properties/range are three copies of the same object (min, max, both required), differing only in description. They sit inside allOf children that already reference signal-listing, and that duplication is the only reason three Range classes come out. If the spec defines it once, as core/signal-range.json referenced from signal-listing, with the two copies removed, it validates the same, so it's fine in a minor. I've suggested it on #8866, which already edits all three objects. After that, fix_signal_listing_range_subclasses, its helpers, the manifest entry and tests/test_signal_listing_range_reuse.py can go, and aliases.py:178 points at the generated SignalRange.
As it stands at the new head, the tree also keeps two distinct types for one wire object: get_signals_response.Range is not signal_listing.Range, and tests/test_signal_listing_range_reuse.py:295 asserts that as intended.
A smaller point, only if the pass stays: "fail loudly when a copy survives" is narrower than described. scripts/post_generate_fixes.py:5238-5242 skips a module whose copy the generator renamed (for example Range1), and the survival check at 5281-5282 runs right after the only class Range was replaced, so it can't fire. The guards that work are the divergence raise and the real-tree test.
Gets the SDK ready for the rc.0 regeneration against adcontextprotocol/adcp#8866, which opens the accidentally closed 3.3 schemas.
Problem
fix_signal_listing_range_subclasses(inscripts/post_generate_fixes.py) exists because a class that extendscore/signal-listing.jsonredeclaresrange. The generator gives that redeclaration its ownRangeclass. It's identical toSignalListing.Rangebut a different type, so the subclass'srangeno longer has its parent's type, and aSignalListing.Rangedoesn't validate there.The pass matched the generated class text of two named files,
extra='forbid'included. That text had already drifted onnext: the generator emitsStrictFloat, and the descriptions changed. So the duplicate incore/wholesale_feed_event.py(class Signal(SignalListing)) was never deleted, and the pass still reported success.The post-generation manifest didn't catch it either:
remove_imports_shadowed_by_a_local_classthen removed that now-shadowed import, so that pass counted as firing too;#8866 opens
core/signal-listing.json#/properties/range(extra='allow'), which would have kept the text match missing.Change
The pass no longer names files or matches text:
Scan: it checks every generated module for a class that subclasses
SignalListingand a top-levelRange.Compare: each such
Rangeis checked againstSignalListing.Rangeby what it validates: bases,model_config, field types,Fieldconstraints and defaults. Descriptions are ignored.On a match: it deletes the copy and adds
Rangeto the module's existingfrom …signal_listing import SignalListing.It raises
RuntimeErrorwhen:SignalListingin a formRangecan't be added to;A silent miss is now a regeneration failure.
Schemas that don't subclass are left alone. A module that declares its own
Rangewithout subclassingSignalListingkeeps it, as every schema keeps its own classes.signals/get_signals_response.pyinlined its signal in 3.3 and has no parent type to match, so it's unchanged, along withadcp.types.domains.signals.RangeandSignalCoverageRange.Manifest:
remove_imports_shadowed_by_a_local_classis now a recorded no-op, with its reason. Its only target was the shadow the broken pass created. It stays as a guard.Generated diff (3.3.0-beta.5)
core/wholesale_feed_event.py: the duplicateRangeclass is removed, andfrom .signal_listing import Range, SignalListing.wholesale_feed_event.Signal.rangenow has the same type asSignalListing.range.wholesale_feed_event.Rangeis still importable, and isSignalListing.Range.StrictFloatmin/max.docs/shared-type-names.md:Rangehas one variant fewer.SCHEMA_DELTAS.mdis the regenerated delta.coredomain root never boundRange, because several core schemas declare it.Verification
generate_types.py --check: up to date. Without the manifest update, the manifest grading fails on the shadow pass.Against #8866's schema bundle (CI artifact
schema-pr-bundle-9c1c52de…, #8866's current head, SHA-256 verified): I extracted it into a scratch worktree's cache, ranfix_schema_refs.py, thengenerate_types.py.wholesale_feed_event.Range is SignalListing.Range, and both are open (extra='allow').get_signals_responsekeeps its ownRange, which is also open.Tests: new
tests/test_signal_listing_range_reuse.py, 19 tests. It covers:model_configshape, on subclass modules in two domains;Range(untouched, even when it diverges);Range, and a missing listingRange;The pre-fix pass, run on the beta.5 and #8866 fixtures, left
class Rangein place without raising and reported success.Local: the full suite passes (21,962 passed, 2,978 skipped), including
test_export_surface_is_derived.pyandtest_public_api.py.make typecheckpasses.Not in scope
Two other passes hard-code
extra='forbid', and neither reads it from the source:fix_verify_brand_claim_models(the JWS envelope;core/response-payload-jws-envelope.jsonstays closed in #8866, so it's consistent today);restore_trusted_match_compatibility_aliases(the deprecated 3.1.8TmpxMacrocompat model, which has no 3.3 source).🤖 Generated with Claude Code